Fix multithreaded wasm crash (solves #164)#165
Conversation
|
The CI seems to have failed due to unrelated reason. Can someone rerun? |
josephlr
left a comment
josephlr
left a comment
There was a problem hiding this comment.
Looks good, a few minor changes. Also, now that we are depending on js-sys can you add a TODO where we use Global and Self_ to replace that functionality with js_sys::global()
| struct BrowserCryptoContext { | ||
| crypto: BrowserCrypto, | ||
| // A temporary buffer backed by browser memory to avoid multithreaded wasm exception. See issue #164 | ||
| buf: Uint8Array, | ||
| } | ||
|
|
There was a problem hiding this comment.
Instead of having this structure, we could just do:
enum RngSource {
Node(NodeCrypto),
Browser(BrowserCrypto, Uint8Array),
}and then add the comment about using the temporary buffer when we actually use the temp buffer:
RngSource::Browser(crypto, buf) => {
// getRandomValues does not work with all types of WASM memory, so
// we initially write to browser memory to avoid an exception.
for chunk in dest.chunks_mut(BROWSER_CRYPTO_BUFFER_SIZE) {
...
}
}There was a problem hiding this comment.
Sorry for the late response, but do you still want me to fix this? Since you force pushed to my branch and already approved it
There was a problem hiding this comment.
Yes, fixing this would be preferable (I can also do it when i have time).
| let buf = Uint8Array::new_with_length(BROWSER_CRYPTO_BUFFER_SIZE as u32); | ||
|
|
||
| let ctx = BrowserCryptoContext { crypto, buf }; | ||
|
|
||
| return Ok(RngSource::Browser(ctx)); |
There was a problem hiding this comment.
This then becomes:
let buf = Uint8Array::new_with_length(BROWSER_CRYPTO_BUFFER_SIZE as u32);
return Ok(RngSource::Browser(crypto, buf));
josephlr
force-pushed
the
wasm_thread_issue
branch
from
7e44e40 to
2831ecc
Compare
Ya that should be fine, once we make sure this works, I'll backport it (maybe once we also cleanup the |
Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
Solves rust-random#164 for the v0.1 branch Signed-off-by: Joe Richey <joerichey@google.com>
Signed-off-by: Joe Richey <joerichey@google.com>
2 participants
This solves #164 by using a preallocated
Uint8Arrayfor callingcrypto.getRandomValues.The
stdwebimplementation seems fine, as it already usesUint8Array.Should this fix be backported to
0.15? There are a lot of crates that still use that version.